Ignore all lines of subsequent hunks until last one is found #602
Conversation
Git version 2.11.1+ introduced extra lines into the subsequent hunk sections for incremental blame output. The documentation notes that parsers of this output should ignore all lines between the start and end for robust parsing.
Codecov Report
@@ Coverage Diff @@
## master #602 +/- ##
=========================================
+ Coverage 94.48% 94.5% +0.02%
=========================================
Files 63 63
Lines 9958 9961 +3
=========================================
+ Hits 9409 9414 +5
+ Misses 549 547 -2
Continue to review full report at Codecov.
|
| orig_filename = value | ||
| # Discard all lines until we find "filename" which is | ||
| # guaranteed to be the last line | ||
| while True: |
There was a problem hiding this comment.
I believe this loops break-condition is not clearly defined just now.
Do you think 'end-of-stream' could be another one?
What do you think of something like this:
for line in stream:
tag, value = line.split(b' ', 1)
if tag == b'filename':
orig_filename = value
break
# Finally handle the case of the stream being exhausted too earlyThere was a problem hiding this comment.
I've added a comment in 77b20be to be more explicit about an unexpected EOF condition. With a for loop we'd need to raise that exception ourselves in an else block so I think the while loop works better here?
There was a problem hiding this comment.
Ah, I see now! I didn't know that next throws - iterators do that, right. Have been doing Rust for too long ;).
|
Thanks a lot! In addition to the comment, how feasible/easy is it for you to add a test based on the kind of input you are handling? I believe there are fixtures for this already, maybe it's possible to adjust one too. |
|
@Byron – yep I think that sort of test should be easy to add with an extra fixture that has the new style git output. I'll add that as soon as I get the time :) |
|
@ghickman Great, I will be waiting for that one then - by now github sends emails on new commits, so I should see it and merge shortly thereafter. |
| @@ -0,0 +1,33 @@ | |||
| 82b8902e033430000481eb355733cd7065342037 2 2 1 | |||
There was a problem hiding this comment.
Thanks a lot! I am ready to merge!
One last question though: should there or should there not be empty lines in the fixture?
There was a problem hiding this comment.
I don't think there should be empty lines in the fixture.
If it helps I generated the fixture with git blame --incremental -p 82b8902e033430000481eb355733cd7065342037 -- AUTHORS > git/test/fixtures/blame_incremental_2.11.1_plus (which I believe matches how blame_incremental was generated).
|
Awesome! Thanks a lot for this one! |
Git version 2.11.1+ introduced extra lines into the subsequent hunk
sections for incremental blame output. The documentation notes that
parsers of this output should ignore all lines between the start and end
for robust parsing.